-
Notifications
You must be signed in to change notification settings - Fork 21.4k
cmd/keeper: use the ziren keccak precompile #32816
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
cmd/keeper/go.ziren.mod
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to self: this file should no longer be needed and, once I confirmed that, it should be removed.
So the approach here is, you want to make a custom version of the I don't think it's a solid approach, and it seems like misuse of the toolchain. If we want to make the keccak implementation replaceable, we need to add hooks across the codebase that allows us to set it. |
4bea991
to
cbf86b4
Compare
It's a standard use of the toolchain, and is used all over the place. Please expand why you think it's not solid? On the other hand, creating hooks all over the place means a lot more code, longer function signatures, reduced readability and a larger diff. I'm not convinced this is a better approach. |
What I meant by 'misuse' is the practice of replacing a package with a module. I'm not sure this is really a supported feature of the toolchain. https://go.dev/ref/mod#go-mod-file-replace
So the thing that's being replaced must be a required module. If we want to use the module replacement approach as a way of replacing code in go-ethereum, we would probably need to extract the crypto bits into their own module first, then replace that module. Your approach here also relies on defining a ziren-specific module file
and then hope it builds. This is ugly because none of the usual methods for maintaining Overall, I feel module replacements are not the right tool for the job here. We have two other options:
|
Uses the go module's
replace
directive to delegate keccak computation to precompiles.This is still in draft because it needs more testing. Also, it relies on a PR that I created, that hasn't been merged yet.